Add in-VM OTLP telemetry export sink#308
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
archandatta
force-pushed
the
archand/kernel-1213/otel-export
branch
2 times, most recently
from
fa0efbe to
63c9a7d
Compare
Adds an optional in-VM OTLP/HTTP export sink that converts browser telemetry events into OTLP log records and forwards them to a configured endpoint, alongside the existing S2 sink. Gated on OTLP_RELAY_ENDPOINT; no behavior change when unset. - Converter maps each telemetry envelope to an OTLP log record: event type to EventName (also mirrored to kernel.event.type for backends that drop it), structured JSON body, derived severity, and kernel.* attributes. Network and console fields are promoted to semantic-convention attributes. Screenshot and monitor categories are excluded from export. - Sink mirrors S2StorageWriter: an independent ring reader feeds records through the OTel log SDK (otlploghttp + batch processor), running independently of the S2 sink. Export failures surface via a logging exporter wrapper. - Config and startup wiring, plus a dev collector config for local testing.
archandatta
force-pushed
the
archand/kernel-1213/otel-export
branch
from
63c9a7d to
5be369e
Compare
rgarcia
left a comment
rgarcia
left a comment
There was a problem hiding this comment.
reviewed — overall looks good. the converter/sink split is clean, mirroring the S2 writer lifecycle was the right call, and the e2e test decoding real OTLP protobuf off the wire is solid. two naming findings:
server/cmd/config/config.go:54-56— env var naming: consider prefixing with something browser-telemetry-specific (e.g.BTEL_*) so these aren't confused with standardOTEL_*env vars that would control kernel-images-api's own telemetry; alsoRELAYfeels overly specified since the endpoint isn't necessarily a relay (a collector in dev)server/cmd/config/config.go:60-62— nit: field names should beInstanceJWT/InstanceName/MetroName— they're platform identity envs, not OTLP-specific
Sayan-
left a comment
Sayan-
left a comment
There was a problem hiding this comment.
overall lgtm! left a few comments inline. broader review attached below.
part of me wonders if we should expose an enable/disable for exporting in the api but I don't think we should solve that in this PR
Opus Semantic Review
Contract assumption: forwarding destination coupled with vm lifecycle; exporting best effort.
A. Lifecycle & availability
| # | Property | Why | Owner | Status | Evidence |
|---|---|---|---|---|---|
| A1 | Init failure must not crash the browser API | Optional sink shouldn't take down the VM | main.go wiring |
FAIL | main.go:145-148 os.Exit(1) on Start err. Low trigger (lazy otlploghttp.New), but real |
| A2 | Transient runtime export errors don't stop the sink | Endpoint flaps must self-heal | otlpStorage+StorageWriter |
PASS | Emit async non-blocking; processResult swallows+logs (eventsstorage.go:81); Run returns only on ctx err |
| A3 | Read loop lives whole VM lifecycle | [1] always-on requirement | StorageWriter.Run |
PASS | Exits only on reader.Read ctx cancel (ringbuffer.go:146) |
| A4 | ~Zero cost when idle | Runs on every vanilla VM | ringBuffer.Read |
PASS | Parks on <-wake (ringbuffer.go:145-149) |
B. Delivery semantics
| # | Property | Why | Owner | Status | Evidence |
|---|---|---|---|---|---|
| B1 | Loss under pressure is safe (drops, never blocks producers or grows unbounded) | A stalled endpoint must not stall capture or OOM the VM | ring + batch queue | PASS | Best-effort by design (matches PR's deferred durability). Enqueue non-blocking, drops oldest (batch.go:305-317); ring evicts oldest. Just declare best-effort as the contract |
| B2 | Flush buffered records on graceful shutdown | Don't lose tail on SIGTERM | OTLPStorageWriter.Stop |
PASS | Stop→wait done→Drain→provider.Shutdown (main.go:333-339). SIGTERM path only |
C. Data correctness
| # | Property | Why | Owner | Status | Evidence |
|---|---|---|---|---|---|
| C1 | Timestamps wall-clock | Suspend-skew correctness | converter | PASS | time.UnixMicro(ev.Ts); Ts wall-clock contract (event.go:79), defaulted in publishLocked |
| C2 | Per-record payload bounded | Keep under relay body cap | EventStream.Publish |
PASS | truncateIfNeeded ~1MB before ring (eventstream.go:37) |
D. Auth & security
| # | Property | Why | Owner | Status | Evidence |
|---|---|---|---|---|---|
| D1 | Authenticated export | Relay must trust the VM | main.go headers |
PASS | Bearer KERNEL_INSTANCE_JWT (main.go:131-133) |
| D2 | Secrets not logged | Token leak | config.LogValue |
PASS | JWT + S2 token redacted (config.go:66-97) |
E. Observability & isolation
| # | Property | Why | Owner | Status | Evidence |
|---|---|---|---|---|---|
| E1 | Export failures observable | Silent failure looks like healthy idle | loggingExporter + otel global logger |
FAIL | Export errors + ring drops → app slog (no metric). Queue-overflow drops → otel global logger, never wired (otel.SetLogger absent) → silently discarded |
| E2 | OTLP failure isolated from S2 | One sink must not degrade the other | separate writers + non-blocking emit | PASS | Independent readers/pipelines; Enqueue non-blocking → stalled endpoint can't backpressure ring or S2 |
Scorecard
- PASS (10): A2, A3, A4, B1, B2, C1, C2, D1, D2, E2
- FAIL (2): A1, E1
Blocking Summary
A1 (don't os.Exit) · E1 (surface failures in logs as aggregate or by window).
| S2Stream string `envconfig:"S2_STREAM" default:""` | ||
|
|
||
| // OTLP telemetry export. OTLPEndpoint (host[:port]) must be set to enable | ||
| // the OTLP sink, which forwards telemetry events to the metro-api relay (or |
There was a problem hiding this comment.
should drop refs to metro-api
| // otlpSeverity maps an event type to an OTLP severity. It is a deliberately | ||
| // coarse, best-effort mapping keyed off the producer's event-type string | ||
| // conventions (the literal "console_error" and the "_failed" suffix), not the | ||
| // generated event-type constants: those live in the producer package | ||
| // (cdpmonitor), which this package cannot import without a cycle. If those type | ||
| // names change, update this mapping in lockstep. |
There was a problem hiding this comment.
this seems fine as a starting point. I think it would be helpful to mux on service crash and oom kill events as well
| logger log.Logger | ||
| } | ||
|
|
||
| func newOTLPStorage(ctx context.Context, cfg OTLPConfig, log *slog.Logger) (*otlpStorage, error) { |
There was a problem hiding this comment.
I think this is fine as a starting point. I'd like for us to consider how much of these we want to control externally
| InstanceName: config.OTLPInstanceName, | ||
| Metro: config.OTLPMetro, | ||
| }, slogger) | ||
| if err := otlpWriter.Start(ctx); err != nil { |
There was a problem hiding this comment.
I think this makes sense wrt parity to s2. I'd guess ~some events will race with any intermediate relay having the information required to make routing decisions. may need to reshape depending on what we want to guarantee
Rename export env vars from OTLP_RELAY_* to BTEL_OTLP_* so they don't collide with the standard OTEL_* vars that configure the API server's own telemetry, and drop the metro-api-specific "relay" naming (dev points at a collector). Rename the platform-identity config fields to InstanceJWT / InstanceName / MetroName since they aren't OTLP-specific. Default export path is now the standard /v1/logs. Make sink startup non-fatal: an optional best-effort exporter failing to start must not take down the browser, so log and continue instead of os.Exit(1). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The OTel log SDK reports records dropped under sustained backpressure via its global logger at logr V(1). That logger is a no-op until wired, and even once wired it renders just below slog Info, so a default Info handler would still swallow it. Route the SDK global logger to a handler that admits that level when export is enabled, so drops surface instead of being silently discarded (which looked identical to a healthy idle sink). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
TestLoad's expected structs didn't account for the OTLPPath and OTLPServiceName defaults that Load applies, so it failed once those defaults were in place. Assert the applied defaults. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ff3f848. Configure here.
| // capped at ~1MB each at publish, so a small batch keeps typical requests | ||
| // under a common 10MB body cap. This is a count bound, not a byte bound: | ||
| // byte-based batching (the real guard against many large records) is deferred. | ||
| const defaultOTLPMaxBatchRecords = 200 |
There was a problem hiding this comment.
Export batch exceeds size cap
Medium Severity
defaultOTLPMaxBatchRecords is 200 while the adjacent comment targets ~10MB HTTP bodies with envelopes capped near 1MB each. A full batch can be tens or hundreds of megabytes of OTLP payload, so exports are likely rejected by relays or collectors. The sink logs failures via loggingExporter but does not retry ring events, so telemetry is dropped under normal high-volume browser sessions.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ff3f848. Configure here.
3 participants
Summary
BTEL_OTLP_ENDPOINT; no behavior change when unset.EventName(mirrored to akernel.event.typeattribute for backends that dropEventName), a structured JSON body, derived severity, andkernel.*attributes. High-value network/console fields are promoted to semantic-convention attributes (http.response.status_code,url.full,http.request.method). Screenshot and monitor categories are excluded.S2StorageWriter: an independent ring reader feeds records through the OTel log SDK (otlploghttp+ batch processor), running independently of S2.Config (
BTEL_OTLP_*)BTEL_OTLP_ENDPOINT(host[:port]; presence enables the sink),BTEL_OTLP_PATH(default/v1/logs),BTEL_OTLP_INSECURE,BTEL_OTLP_SERVICE_NAME(defaultkernel-browser).KERNEL_INSTANCE_JWT,INST_NAME,METRO_NAME.BTEL_OTLP_PATH=/otlp-relay/v1/logs.Review feedback addressed
OTLP_RELAY_*toBTEL_OTLP_*so they don't collide with the standardOTEL_*vars that configure the API server's own telemetry, and dropped the relay-specific naming (dev points at a collector).InstanceJWT/InstanceName/MetroName(they aren't OTLP-specific).Testing
go build ./...,go vet, and the server unit suite are green.Scope and follow-ups
BTEL_OTLP_ENDPOINTon the VM (not yet wired), gated on a resolved destination so we don't generate work with nowhere to land.Note
Medium Risk
New async telemetry egress path and dependency bump; misconfiguration or relay outages affect observability but are designed not to crash the browser VM on OTLP start failure.
Overview
Adds an optional in-VM OTLP/HTTP export sink for browser telemetry, parallel to the existing S2 writer. When
BTEL_OTLP_ENDPOINTis set, the API reads the same event ring, converts envelopes to OTLP log records, and batches them via the OTel log SDK (otlploghttp). No change when the endpoint is unset.New
BTEL_*config covers endpoint, path, insecure mode, service name, and reuses platform identity (KERNEL_INSTANCE_JWT,INST_NAME,METRO_NAME) for export headers and resource attributes. Startup wires OTel’s diagnostic logger so batch-queue drops are visible; OTLP start failure is best-effort (logs and continues) unlike S2, which exits on start failure. Shutdown drains and flushes the OTLP writer after HTTP servers stop.The converter maps events to OTLP logs with structured JSON bodies,
kernel.*metadata, mirroredkernel.event.type, coarse severity from event type, and promoted network/console fields (http.*,url.full). Screenshot and monitor categories are excluded from export.Unit tests cover conversion and an end-to-end sink test against a fake OTLP receiver;
go.modgains OTel log exporter dependencies.Reviewed by Cursor Bugbot for commit ff3f848. Bugbot is set up for automated code reviews on this repo. Configure here.